@nitaliano @kristfal question regarding expressions, currently we treat value an expression if it's an array and the first item is a string. The issue is with attributes like textFont, they supposed to be an array of text, we need to diferrentiate string array literals from expressions.

So the question is that how do we figure out that

This is a value array of strings:

{ 
   ...,
   textFont: ["Open Sans Regular","Arial Unicode MS Regular"]
}

but this is an expression:

{
   ...
   textFont: ['get', 'font']
}

I see the following solutions:

1. we require expressions to be constructed via special syntax:

   { ... textFont: expression('get','font') ...}

2. we require string array literals to be cosntructed via special syntax:

   { ... textFont: literal('Open Sans Regular', 'Arial Unicode MS Regular') ...}

3. we implement isExpression (this, and we only treat arrays expression if they start with a string, this string is one of known expression operator. The js and c++ mapbox has this, but they have a list of expressions from expressionsRegistry and we don't seems to have access to that. So we might miss some usefull expressions...

Very good question, and good work on this!

As far as developer ergonomics, I think alternative 3 is the strongest. I sense that option 1 or 2, or both combined, will likely be a root cause for questions from devs coming from both native and JS backgrounds.

Yes, we likely do need to maintain a list of supported expression evaluators that are the lowest common denominator of iOS and Android SDKs. However, I think the cost of maintaining this list is lower than the time spent writing documentation and responding to questions regarding the implementation.

If there is a way to extract supported expressions from the gl-js lib, that would be great. On the flip side, it would also require us to keep a close eye on versioning as the JS side often leads in feature support over the native SDKs.

I’m on mobile now, so I haven’t looked at at how we solve it currently, but iirc, we do maintain a list of supported style operators for this repo specifically. I assume something similar for expressions kinda makes sense. @nitaliano built this afaik, so I believe he may be better at giving an opinion about this.

Thanks again for your contributions!

The GeoJSON Source example shows a Failed prop type: Invalid prop 'style' supplied to 'FillLayer' warning. I checked the definition of the FillLayer and the style prop looks like it's defined just fine. Any idea what that's about?

The GeoJSON Source example shows a Failed prop type: Invalid prop 'style' supplied to 'FillLayer' warning. I checked the definition of the FillLayer and the style prop looks like it's defined just fine. Any idea what that's about?

you can see the check the allowed styles for FillLayer bellow, i assume the style has not allowed keys:

https://github.com/mfazekas/maps/blob/571b3b6724f5dd5563560cb5127e1115ce37177c/javascript/utils/styleMap.js#L52-L151

In particular fillAntialias bool doesn't seems to be allowed by the style spec, but i don't think it's related to this branch does it?

You're right, that's the issue, I missed that.

@nitaliano is the code generation correct? According to the style spec file, fill-antialias is supposed to be a boolean (and it has data-constant as it's property-type, not sure if that's relevant), but in the autogenerated code it's treated as an expression. Is that right? If so, I guess the example app needs to change.

As far as developer ergonomics, I think alternative 3 is the strongest. I sense that option 1 or 2, or both combined, will likely be a root cause for questions from devs coming from both native and JS backgrounds.

@kristfal actually my question was a bit rushed. It sounds that both on ios and java passing a fontStack (eg string array) to textFont works. Because either NSExpression expressionWithMGLJSONObject- objc and Expression.raw - java, handles this for us or expressions are not evaluated for textFont. In any case both ios and android supports ['lateral', [...]] too should someone needs to force something to literal. Right now i'm treating every array where the first item is a string as an expression. We can improve on that later if needed.

It might make sense to merge the branch sooner than later. There are stuff to be cleaned up like MarkerViews on android, android camera cleanup, docs, etc.

Great!

Awesome work on this. I see you’ve been working weekends, so thanks a lot man.

If things are stable in terms of crashes, I think it makes sense to merge. I haven’t run this branch myself on our prod repo, but I’m still keen on getting it into master asap. We can label this as a prerelease to communicate it clearly with the community.

Once it is in and stability is verified/good, I think we should publish on NPM and deprecate the old repo.

@kristfal we're using it in our app we're developing so base part of it is stable (Map, Camera, Symbol, FillLayer, Offline both on android+ios). I have no doubt that there are instable/non-consitent parts.

My concern is that if we wait for this branch to be fully stabilized in every aspect, changes will appear on master, and merging will delay stuff even more.

@mfazekas Thank you for your work on this. I share your sentiments on not waiting too long to get this merged.

Hey again @mfazekas, just to clarify: Yes, I think we should merge sooner than later. Do you have ownership to do so? If not, would you like me to merge now or wait?

After that merge, we tag it as a prerelease and if there are issues, we'll fix as we go forward.

Added you as a collaborator @mfazekas and merged #3 (which caused conflicts here), feel free to merge to master at your own leisure.

somehow i've got this closed by mistake

@kristfal i'm not a Member of react-native-mapbox, but i don't seems to have write access for the maps project, so i cannot merge. Please merge.

@mfazekas @kristfal Testing out the new merge a little this evening. I'm getting a behavior where my initial map zoom and initial center coordinates are not working.

      <Mapbox.MapView
        zoomLevel={8}
        centerCoordinate={[-111.8678, 42.2866]}
        style={styles.map}
        showUserLocation={true}
      >

Map loads at zoom 0 as show in the screenshot:

The pre-merge code loads zoom and center coords correctly.

Do you think this is related to this merge in some way? Perhaps not? Not sure why that stopped working.

@systemlevel Hey, thanks for kicking the tires on the post-merge repo. I plan on doing the same sometime in the next few days.

Given master finally has all this new code merged in, maybe this should be a new issue that references this pull request?

@BigSassy Thank you good idea I'll create an issue regarding this. I've got a few more things to note as well as I go through this. Looking forward to your feedback as well. Please let us know anything you find.